Implement context.destination.address/port for DB spans #665
Conversation
…an embedded DB (Full FW)
…n embedded DB (.NET Core)
src/Elastic.Apm.EntityFrameworkCore/EfCoreDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
| internal const int MaxCacheSize = 100; | ||
|
|
||
| private readonly IApmLogger _logger; | ||
| private readonly ThreadLocal<Dictionary<string, Destination>> _cache = |
There was a problem hiding this comment.
I am surprised by using ThreadLocal here. Diagnostic source listeners are executed in the thread from the thread pool, which is not the same each time. I run a small snippet to check it and got the following results (it was release configuration):
Thread Id: 7, Is background: True, Is from thread pool: True
Request '6' started: { "isMaster" : 1, "client" : { "driver" : { "name" : "mongo-csharp-driver", "version" : "2.9.2.0" }, "os" : { "type" : "Windows", "name" : "Microsoft Windows 10.0.18362", "architecture" : "x86_64", "version" : "10.0.18362" }, "platform" : "
.NET Core 3.0.1" }, "compression" : [] }
--- --- --- --- --- --- --- --- ---
Thread Id: 8, Is background: True, Is from thread pool: True
Request '6' succeeded for isMaster in 14.1601ms with { "ismaster" : true, "maxBsonObjectSize" : 16777216, "maxMessageSizeBytes" : 48000000, "maxWriteBatchSize" : 100000, "localTime" : ISODate("2019-12-20T10:36:57.418Z"), "logicalSessionTimeoutMinutes" : 30, "co
nnectionId" : 8, "minWireVersion" : 0, "maxWireVersion" : 8, "readOnly" : false, "ok" : 1.0 }
--- --- --- --- --- --- --- --- ---
Thread Id: 8, Is background: True, Is from thread pool: True
Request '7' started: { "buildInfo" : 1 }
--- --- --- --- --- --- --- --- ---
Thread Id: 8, Is background: True, Is from thread pool: True
Request '7' succeeded for buildInfo in 1.8988ms with { "version" : "4.2.2", "gitVersion" : "a0bbbff6ada159e19298d37946ac8dc4b497eadf", "modules" : [], "allocator" : "tcmalloc", "javascriptEngine" : "mozjs", "sysInfo" : "deprecated", "versionArray" : [4, 2, 2, 0
], "openssl" : { "running" : "OpenSSL 1.1.1 11 Sep 2018", "compiled" : "OpenSSL 1.1.1 11 Sep 2018" }, "buildEnvironment" : { "distmod" : "ubuntu1804", "distarch" : "x86_64", "cc" : "/opt/mongodbtoolchain/v3/bin/gcc: gcc (GCC) 8.2.0", "ccflags" : "-fno-omit-fr
ame-pointer -fno-strict-aliasing -ggdb -pthread -Wall -Wsign-compare -Wno-unknown-pragmas -Winvalid-pch -Werror -O2 -Wno-unused-local-typedefs -Wno-unused-function -Wno-deprecated-declarations -Wno-unused-const-variable -Wno-unused-but-set-variable -Wno-missing
-braces -fstack-protector-strong -fno-builtin-memcmp", "cxx" : "/opt/mongodbtoolchain/v3/bin/g++: g++ (GCC) 8.2.0", "cxxflags" : "-Woverloaded-virtual -Wno-maybe-uninitialized -fsized-deallocation -std=c++17", "linkflags" : "-pthread -Wl,-z,now -rdynamic -Wl,--
fatal-warnings -fstack-protector-strong -fuse-ld=gold -Wl,--build-id -Wl,--hash-style=gnu -Wl,-z,noexecstack -Wl,--warn-execstack -Wl,-z,relro", "target_arch" : "x86_64", "target_os" : "linux" }, "bits" : 64, "debug" : false, "maxBsonObjectSize" : 16777216, "st
orageEngines" : ["biggie", "devnull", "ephemeralForTest", "wiredTiger"], "ok" : 1.0 }
--- --- --- --- --- --- --- --- ---
Thread Id: 8, Is background: True, Is from thread pool: True
Request '8' started: { "getLastError" : 1 }
--- --- --- --- --- --- --- --- ---
Thread Id: 8, Is background: True, Is from thread pool: True
Request '8' succeeded for getLastError in 1.3274000000000001ms with { "connectionId" : 8, "n" : 0, "syncMillis" : 0, "writtenTo" : null, "err" : null, "ok" : 1.0 }
--- --- --- --- --- --- --- --- ---
Thread Id: 8, Is background: True, Is from thread pool: True
Request '9' started: { "find" : "people", "filter" : { }, "$db" : "diagnosticsource", "lsid" : { "id" : CSUUID("654c3f97-6643-4fa5-828e-ab474f8807fa") } }
--- --- --- --- --- --- --- --- ---
Thread Id: 7, Is background: True, Is from thread pool: True
Request '9' succeeded for find in 8.997200000000001ms with { "cursor" : { "firstBatch" : [{ "_id" : ObjectId("5dfca35bb523d240ef428a13"), "name" : "Joe Smith", "email" : "[email protected]", "age" : 40, "admin" : false }, { "_id" : ObjectId("5dfca35bb523d240ef42
8a14"), "name" : "Jen Ford", "email" : "[email protected]", "age" : 45, "admin" : true }], "id" : NumberLong(0), "ns" : "diagnosticsource.people" }, "ok" : 1.0 }
--- --- --- --- --- --- --- --- ---
Thread Id: 7, Is background: True, Is from thread pool: True
Request '10' started: { "$db" : "diagnosticsource", "lsid" : { "id" : CSUUID("654c3f97-6643-4fa5-828e-ab474f8807fa") } }
--- --- --- --- --- --- --- --- ---
Thread Id: 8, Is background: True, Is from thread pool: True
Request '10' failed with 'MongoDB.Driver.MongoCommandException: $db command failed'
--- --- --- --- --- --- --- --- ---
So, using ThreadLocal is less efficient for a caching scenario in our use case than ConcurrentDictionary. What was your motivation?
There was a problem hiding this comment.
Just chiming in quickly: I haven't looked at the whole PR yet, just parsed it a little bit last evening and I also had the same question... I also thought that I'd just use static ConcurrentDictionary, unless there is a good reason not to.
There was a problem hiding this comment.
It's an interesting point - I'll compare performance characteristics of using ThreadLocal vs. ConcurrentDictionary.
There was a problem hiding this comment.
I went through it. Nice! 👍
As a follow up PR it might be worth considering to improve DbConnectionStringParser cache strategy (currently it's just caches up to 100 first connection strings) - maybe LRU cache would be better?
I agree with this, the current caching isn't optimal, but as you say, totally fine in a follow up PR.
Two things I'd think about:
- The
ThreadLocalthing mentioned here - Implement context.destination.address/port for DB spans #665 (comment) - I strongly think that caching pro thread is not optimal and probably caching globally or proDbConnectionStringParserinstance would be easier to understand and not worse in terms of performance - especially if we think about memory overhead. - I'd reconsider the hard cast in
EfCoreDiagnosticListener. I saw this discussion: Implement context.destination.address/port for DB spans #665 (comment) and I'd probably stop by makingDbConnectionStringParseran instance ofDbSpanCommon, but I'd leave it static - that's less dangerous than the cast. - and the expression bodied members :)
Furthermore, I feel #667 would be extremely helpful - running ExternalDbTests took me very long, and I only tried MSSQL. But all that is better as follow-up PS.
src/Elastic.Apm.EntityFrameworkCore/EfCoreDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
src/Elastic.Apm.EntityFrameworkCore/EfCoreDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
| span.Subtype.Should().Be(externalDbType.SpanSubtype); | ||
| span.Action.Should().Be(ApiConstants.ActionQuery); | ||
| span.Context.Db.Type.Should().Be(Database.TypeSql); | ||
| span.Context.Destination.Address.Should().Be(connectionDetails.Host); |
There was a problem hiding this comment.
I wanted to give this test a try and it produced a test fail for me with SQL Server by doing:
set ELASTIC_APM_TESTS_MS_SQL_HOST=GREGWINDOWSDEV\SQLEXPRESS
The expected connectionDetails.Host was GREGWINDOWSDEV\SQLEXPRESS, but DbConnectionStringParser.ParseServerValue produced GREGWINDOWSDEV which ends up in span.Context.Destination.Address.
There was a problem hiding this comment.
IIUC SQLEXPRESS part is DB instance name - this test uses default DB instance. I've added a comment clarifying this assumption.
There was a problem hiding this comment.
Ok, now I understand your intent. Unfortunately leaving out \SQLEXPRESS does not make the test pass on my machine - as it seems it is not a default DB instance name and under the generated connection string Data Source=GREGWINDOWSDEV;Initial Catalog=ElasticApmExternalDbTests;User ID=greg;Password=1234 (this password is not a secret ;) ) the database is not available.
Anyways, there is already a follow up issue for this.
|
I saw 1 test is failing in CI: Can't repro locally - I see the build was already re-triggered. If it's green I'm ok with merging it. |
Closes #611.
Implements the remaining part of #611 (context.destination.address/port for DB spans).
As a follow up PR it might be worth considering to improve
DbConnectionStringParsercache strategy (currently it's just caches up to 100 first connection strings) - maybe LRU cache would be better?